-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replaced custom bash script for copy-mjs tool #1366
Conversation
@Andarist Thanks for PR 👍 |
I'm not 100% sure what @leebyron 's philosophy on pulling in 3rd-party scripting tools is, and whether it's OK to add this dev dependency. My gut says it likely is OK, but I probably will wait until after the 14.0 release to fully consider this PR: it won't be a breaking change regardless, and I want to reduce the moving parts to just code/node-version changes as much as possible. But thank you @Andarist for contributing to cleaning up hackish shell scripts! |
@Andarist @mjmahone I like how it's done in https://github.com/facebook/react/blob/52fbe7612e0527b8c86decac519c344626f6bd72/package.json#L108 |
I agree that react's script is really nicely written. It's usually just way more time consuming to setup it like this (initially). I don't believe that using sync-only functions is the way to go, as requirements arise its more future-proof to use async from the start. My |
While bash can be weird, I'd honestly prefer having fewer dev dependencies just so that we can more easily control this build process in the future, especially since this tool seems to only replace half of this build step instead of all of it. It's likely that we may want to use babel's A js script that runs the whole build like React has set up may be easier to work on than our patched together npm and bash scripts.
@Andarist I'd challenge you to question this assumption. I believe what you say is likely true if your intent is to support web servers or other highly parallel operations, but build scripts tend to be highly serial. I seem to remember joliss doing some deep experimentation on this point while building the broccoli build to and found that async added measurable overhead to build time in aggregate. |
I agree it's highly serial - it doesn't mean that whole process has to be synchronous though. Certain steps can be async for perf-boost or just because the author thought it would be a good idea (author of certain step-util/tool that is) |
Closing this PR since:
is implemented in #1841 |
I've packaged this script into a reusable tool to use in another project, thought you might like it here too. It's advantage is that it should run fine on windows too.